Annotation-driven deprecated table config validation on create + update#18411
Annotation-driven deprecated table config validation on create + update#18411xiangfu0 wants to merge 18 commits into
Conversation
327db64 to
e02ba1c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18411 +/- ##
=============================================
+ Coverage 35.23% 63.72% +28.48%
- Complexity 1117 1932 +815
=============================================
Files 3292 3293 +1
Lines 201470 201796 +326
Branches 31316 31398 +82
=============================================
+ Hits 70998 128586 +57588
+ Misses 124206 62878 -61328
- Partials 6266 10332 +4066
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
noob-se7en
left a comment
There was a problem hiding this comment.
1 major error-handling / 2 medium follow-ups.
2fae407 to
fbc259a
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces annotation-driven validation to reject explicit usage of deprecated TableConfig JSON keys on both table/config creation and updates, while preserving backward compatibility for legacy values already stored in ZK by diffing against the raw stored ZNRecord JSON.
Changes:
- Add
@DeprecatedConfig(replacement, since)annotations on deprecated SPI config getters and reflectively discover deprecated JSON paths for validation. - Enforce deprecated-config validation on controller create/update/validate endpoints (with version-aware warning vs error severity) and surface warnings via
deprecationWarnings. - Update builders, tests, and example table-config JSON to use modern fields (ingestion configs,
indexTypes,jsonIndexConfigs, etc.), plus add raw-ZK JSON reconstruction utilities for update diffing.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pinot-spi/src/main/java/org/apache/pinot/spi/config/DeprecatedConfig.java | Adds the new deprecation annotation used as the single source of truth for deprecated JSON keys. |
| pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/DeprecatedTableConfigValidationUtils.java | Implements reflective rule discovery + create/update-time deprecated-key validation with version-aware severity. |
| pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigSerDeUtils.java | Adds toRawJsonNode(ZNRecord) to reconstruct raw stored JSON for byte-faithful update diffing. |
| pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java | Adds getTableConfigZNRecord() helper to fetch raw table config ZNRecord. |
| pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java | Enforces deprecated-config validation on table create/update/validate and returns warnings in responses. |
| pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java | Enforces deprecated-config validation for TableConfigs create/update/validate and returns warnings in responses. |
| pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ConfigSuccessResponse.java | Adds optional deprecationWarnings field to success responses. |
| pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java | Converts deprecated builder setters (segment push + stream configs) into modern ingestion config fields and omits deprecated serialized keys. |
| pinot-spi/src/test/java/org/apache/pinot/spi/utils/builder/TableConfigBuilderTest.java | Tests conversion/omission behavior of deprecated fields in builder output JSON. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java | Annotates deprecated segment push + other deprecated fields with @DeprecatedConfig. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/IndexingConfig.java | Annotates deprecated indexing fields (jsonIndexColumns, streamConfigs, etc.) with @DeprecatedConfig. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java | Marks legacy indexType as deprecated config key for validation while preserving deserialization via @JsonCreator. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/RoutingConfig.java | Annotates deprecated routingTableBuilderName with @DeprecatedConfig. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java | Annotates deprecated upsert booleans with @DeprecatedConfig and NON_DEFAULT inclusion. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/DedupConfig.java | Annotates deprecated dedup booleans with @DeprecatedConfig and NON_DEFAULT inclusion. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/InstanceReplicaGroupPartitionConfig.java | Annotates deprecated nested minimizeDataMovement with @DeprecatedConfig. |
| pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeUtilsTest.java | Adds tests ensuring toRawJsonNode() preserves keys stripped by bean round-trips. |
| pinot-controller/src/test/java/org/apache/pinot/controller/api/resources/DeprecatedTableConfigValidationUtilsTest.java | Adds unit tests for rule discovery, version severity, create vs update diff behavior. |
| pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java | Adds REST-level tests for rejecting deprecated keys on create and on update when newly introduced. |
| pinot-controller/src/test/java/org/apache/pinot/controller/api/TableConfigsRestletResourceTest.java | Adds TableConfigs REST test ensuring deprecated keys are rejected on create. |
| pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java | Updates test table config creation to use modern ingestion config handling. |
| pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManagerTest.java | Migrates tests to use ingestion-config helpers instead of deprecated streamConfigs. |
| pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasePauselessRealtimeIngestionTest.java | Migrates ingestion setup away from deprecated streamConfigs. |
| pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/logicaltable/LogicalTableWithTwoRealtimeTableIntegrationTest.java | Migrates stream config access to ingestion-config utilities. |
| pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/realtime/ingestion/KafkaIncreaseDecreasePartitionsIntegrationTest.java | Refactors test to rely on base-class topic/table wiring rather than manual creation. |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java | Adjusts legacy conversion test to inject deprecated keys directly (since builder now produces modern ingestion config). |
| pinot-tools/src/main/java/org/apache/pinot/tools/BootstrapTableTool.java | Adds a null-guard around batch config maps iteration. |
| pinot-tools/src/main/resources/conf/sample_offline_table_config.json | Updates sample config to modern ingestion fields / removes deprecated keys. |
| pinot-tools/src/main/resources/conf/sample_realtime_table_config.json | Updates sample config to modern ingestion fields / removes deprecated keys. |
| pinot-tools/src/main/resources/examples/batch/airlineStats/airlineStats_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/baseballStats/baseballStats_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/minions/batch/baseballStats/baseballStats_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/clickstreamFunnel/clickstreamFunnel_offline_table_config.json | Removes deprecated keys and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/dimBaseballTeams/dimBaseballTeams_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/fineFoodReviews/fineFoodReviews_offline_table_config.json | Migrates field configs to indexTypes and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/githubEvents/githubEvents_offline_table_config.json | Migrates jsonIndexColumns -> jsonIndexConfigs and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/githubComplexTypeEvents/githubComplexTypeEvents_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/starbucksStores/starbucksStores_offline_table_config.json | Migrates H3 field config to indexTypes and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/testUnnest/testUnnest_offline_table_config.json | Removes deprecated fields and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/dailySales/dailySales_realtime_table_config.json | Removes deprecated keys and migrates to modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/fineFoodReviews/fineFoodReviews_realtime_table_config.json | Removes deprecated keys and migrates to modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/fineFoodReviews_part_0/fineFoodReviews_part_0_realtime_table_config.json | Removes deprecated keys and migrates to modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/fineFoodReviews_part_1/fineFoodReviews_part_1_realtime_table_config.json | Removes deprecated keys and migrates to modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/meetupRsvpJson/meetupRsvpJson_realtime_table_config.json | Migrates jsonIndexColumns -> jsonIndexConfigs. |
| pinot-tools/src/main/resources/examples/stream/upsertJsonMeetupRsvp/upsertJsonMeetupRsvp_realtime_table_config.json | Migrates jsonIndexColumns -> jsonIndexConfigs. |
| pinot-tools/src/main/resources/examples/stream/upsertMeetupRsvp/upsertMeetupRsvp_realtime_table_config.json | Migrates upsert deprecated fields to modern enums; migrates indexType -> indexTypes. |
| pinot-tools/src/main/resources/examples/stream/upsertPartialMeetupRsvp/upsertPartialMeetupRsvp_realtime_table_config.json | Migrates indexType -> indexTypes. |
| pinot-integration-tests/src/test/resources/chaos-monkey-create-table.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-integration-tests/src/test/resources/dimDayOfWeek_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/admin/README.md | Documents modern create payload fields and common migrations away from deprecated keys. |
Comments suppressed due to low confidence (1)
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:238
setSegmentPushType()/setSegmentPushFrequency()are documented as deprecated in their Javadoc, but they are not annotated with@Deprecated(unlike other deprecated builder APIs in this class, e.g.setLLC).
Annotate these methods with @Deprecated so callers get compiler warnings and IDE tooling consistently flags their use.
/**
* @deprecated Use {@code segmentIngestionType} from {@link IngestionConfig#getBatchIngestionConfig()}
*/
public TableConfigBuilder setSegmentPushType(String segmentPushType) {
if (REFRESH_SEGMENT_PUSH_TYPE.equalsIgnoreCase(segmentPushType)) {
_segmentPushType = REFRESH_SEGMENT_PUSH_TYPE;
} else {
_segmentPushType = "APPEND";
}
return this;
}
/**
* @deprecated Use {@code segmentIngestionFrequency} from {@link IngestionConfig#getBatchIngestionConfig()}
*/
public TableConfigBuilder setSegmentPushFrequency(String segmentPushFrequency) {
_segmentPushFrequency = segmentPushFrequency;
return this;
}
5870292 to
3cb2910
Compare
xiangfu0
left a comment
There was a problem hiding this comment.
Found two high-signal deprecation-validation gaps; see inline comments.
3558fc1 to
1173e98
Compare
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal enforcement gap; see inline comment.
a4a4b3f to
e252bed
Compare
xiangfu0
left a comment
There was a problem hiding this comment.
Two validate endpoints still echo the full raw config body on parse failures. Because ControllerApplicationException logs 4xx messages, that leaks stream credentials and other secrets into controller logs; inline details below.
| tableConfigsJson = JsonUtils.stringToJsonNode(tableConfigsStr); | ||
| } catch (IOException e) { | ||
| throw new ControllerApplicationException(LOGGER, | ||
| String.format("Invalid TableConfigs json string: %s. Reason: %s", tableConfigsStr, e.getMessage()), |
There was a problem hiding this comment.
This still reflects the entire tableConfigsStr into both the 400 response and the controller log line (ControllerApplicationException logs 4xx messages). TableConfigs payloads can carry stream credentials, so a malformed /tableConfigs/validate request now leaks secrets into logs. Please match the addConfig() scrub here and keep only e.getMessage().
There was a problem hiding this comment.
Fixed in 05def21: switched to "Invalid TableConfigs json string. " + e.getMessage() to match addConfig. Also reordered /tableConfigs/validate so the permission check runs BEFORE the ZK-reading validateNoDeprecatedConfigs path, mirroring PinotTableRestletResource.checkTableConfig and preventing an unauthenticated caller from probing table existence.
| JsonUtils.stringToObjectAndUnrecognizedProperties(tableConfigStr, TableConfig.class); | ||
| tableConfigJson = JsonUtils.stringToJsonNode(tableConfigStr); | ||
| } catch (IOException e) { | ||
| String msg = String.format("Invalid table config json string: %s. Reason: %s", tableConfigStr, e.getMessage()); |
There was a problem hiding this comment.
Same leak on /tables/validate: parse failures log the full raw table config via ControllerApplicationException, including any stream config credentials in the request body. The new PR already scrubbed similar error paths elsewhere, so this validate endpoint needs the same treatment.
There was a problem hiding this comment.
Fixed in 05def21: scrubbed the raw tableConfigStr from both the 400 response and the controller log. The catch now uses concatenation ("Invalid table config json string. " + e.getMessage()), matching the addTable/copyTable scrub pattern already in this file.
…re severity Introduce @DeprecatedConfig(replacement, since) on SPI getters and replace the manually-maintained rule list in DeprecatedTableConfigValidationUtils with a Jackson-aware reflection walk over TableConfig. Severity is decided by comparing the annotation since against PinotVersion.VERSION major.minor so deprecations from the current release line are warnings and older ones are errors. Wire both create and update paths to validate against deprecated keys. Update paths diff incoming JSON against the byte-faithful stored ZNRecord JSON via the new TableConfigSerDeUtils.toRawJsonNode + getTableConfigZNRecord so legacy values left over on existing tables don't trigger false-positive "newly introduced" errors on every PUT. Surface non-fatal warnings via ConfigSuccessResponse.deprecationWarnings.
- Drop String.format and full request body from "Invalid TableConfigs" error messages; match the pattern used by addConfig (per apache#14404). - Parameterize DeprecatedTableConfigValidationUtilsTest with a @dataProvider over every rule discovered by the annotation walk so test coverage tracks the rule list 1:1.
- copyTable: catch IllegalArgumentException (not IllegalStateException) to keep deprecated-config rejection at 400 instead of falling through to the generic 500 handler. - DeprecatedTableConfigValidationUtils Javadoc: align documented exception type with the actual IllegalArgumentException thrown by validateOnCreate / validateOnUpdate. - @DeprecatedConfig: restrict @target to METHOD only, since the discovery walk only inspects methods. Prevents misleading field-level annotations that would silently never fire. - Stream realtime example configs: change segmentAssignmentConfigMap key from REALTIME to COMPLETED. The map is keyed by assignment type, not table type; SegmentAssignmentStrategyFactory looks it up via assignmentType.toUpperCase() so REALTIME entries were dead.
setSegmentPushType, setSegmentPushFrequency, and setStreamConfigs in TableConfigBuilder were documented as deprecated in their Javadoc but not annotated, so callers got no compiler warning. Add @deprecated and convert the comments to /// Markdown style for consistency.
DeprecatedTableConfigValidationUtils: - Scoped isJacksonDefault skip to apply only when stored value is absent (oldValue == null) so a deliberate flip of an existing non-default value back to the default is still flagged. - Tightened isJacksonDefault numeric checks: short/int/long via longValue(); BigInteger / BigDecimal via dedicated zero comparisons; floating-point via Double.compare so -0.0 is non-default. Avoids the lossy double coercion of arbitrary-precision types. - Tightened textual branch to match Jackson NON_DEFAULT: only explicit null is default for null-default String fields; empty string is a real user value and must be flagged on update. - Added classifySeverity(String, String) test seam so the version- unknown WARNING fallback can be unit-tested. - Updated class-level Javadoc to describe the WARNING fallback when the running Pinot version is unknown (was previously documented as ERROR). PinotTableRestletResource / TableConfigsRestletResource: - Split BAD_REQUEST catches so ZK transient failures (RuntimeException from getTableConfigZNRecord / validate helpers) propagate as 5xx instead of being mis-reported as 400. - Added warn-log + ControllerMeter increment on each new RuntimeException catch so on-call observability is preserved. - Capture JsonNode view of the request body once on update path to avoid double-parse before the deprecation diff. - Removed dead uppercase fallback in subConfigJson; added Javadoc pointing to the deserialization invariant it relies on. - Removed unreleased legacy validateNoDeprecatedConfigs(JsonNode[, String]) shims; one remaining caller (copyTable) inlined to validateOnCreate. ConfigSuccessResponse: - Moved @JsonInclude(NON_EMPTY) from field to getter so empty deprecationWarnings are correctly elided from the wire. - Normalised null _unrecognizedProperties to Map.of(). @DeprecatedConfig SPI annotation: - Restricted @target to METHOD only (matches discovery). - Added "metadata-only" Javadoc note clarifying that the annotation has no runtime serialization effect. Tests: - Parameterized DeprecatedTableConfigValidationUtilsTest over discovered rules, exercising both array-wildcard and map-wildcard JSON shapes. - Added regression coverage for the new isJacksonDefault skip semantics (re-submitted default, deliberate flip, explicit JSON null on stored side, empty string on textual paths). - Added testSeverityFallsBackToWarningWhenCurrentVersionUnknown + testSeverityWithExplicitCurrentVersion locking the WARNING fallback. - Added ConfigSuccessResponseTest for NON_EMPTY behavior + null normalisation. - Added testUppercaseOfflineRealtimeKeysAreIgnoredByJackson in TableConfigsSerializationTest locking the invariant subConfigJson relies on. Quickstart configs (4 stream realtime examples): segmentAssignmentConfigMap key changed from REALTIME to COMPLETED (the map is keyed by assignment type, not table type; SegmentAssignmentStrategyFactory looks it up via assignmentType.toUpperCase() so REALTIME entries were dead).
PUT to a nonexistent table whose body included a deprecated key was
returning a misleading 400 ("Newly introduced deprecated table config
properties are not allowed: ...") because the deprecation diff ran with
oldTableConfigJson==null (which fell through to create-mode) before
the hasTable() check.
- PinotTableRestletResource.updateTableConfig: hasTable check moved
above the deprecation diff so missing table reports 404 cleanly.
- TableConfigsRestletResource.updateConfig: symmetric reorder so
missing table reports the existing "does not exist" 400 instead of
a deprecation 400.
- DeprecatedTableConfigValidationUtils.validateOnUpdate: tightened
contract to reject null oldTableConfigJson so callers cannot
accidentally exercise create-mode under update semantics.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- PinotTableRestletResourceTest.testUpdateMissingTableReports404NotDeprecationError: regression for the recent existence-check reorder; PUT to a missing table whose body has a deprecated key now reports 404 cleanly. - TableConfigsRestletResourceTest.testUpdateConfigRejectsNewlyIntroducedDeprecatedProperty: symmetric to the create-mode rejection, asserts a PUT that introduces a new deprecated key on an existing TableConfigs returns 400 with the deprecated path in the message. - TableConfigsRestletResourceTest.testUpdateMissingTableConfigsReportsNotExistsNotDeprecation: regression for the symmetric reorder in TableConfigsRestletResource; PUT to a missing TableConfigs reports "does not exist" instead of a misleading deprecation 400. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uency
CI surfaced a wider issue with the TableConfigBuilder rewrite that was
bundled with this PR: ~14 realtime integration tests rely on reading
indexingConfig.streamConfigs after building a config via the builder, so
removing the legacy population from build() broke their setUp().
Rather than migrate every test to read from
ingestionConfig.streamIngestionConfig in this PR, remove the three
deprecation annotations whose enforcement requires the builder rewrite,
and revert TableConfigBuilder.build() to its prior shape that populates
the legacy fields. The remaining @DeprecatedConfig annotations
(replicasPerPartition, jsonIndexColumns, indexType, etc.) still validate
on create + update via the diff-based version-aware path.
The dropped deprecations can ship in a follow-up PR after the codebase
migrates fully to the ingestionConfig shape.
- pinot-spi: drop @DeprecatedConfig from getStreamConfigs(),
getSegmentPushType(), getSegmentPushFrequency()
- pinot-spi: revert TableConfigBuilder.build() to populate
validationConfig.{segmentPushType,segmentPushFrequency} and
indexingConfig.streamConfigs
- pinot-controller tests: migrate path-specific assertions from
segmentPushType/streamConfigs to replicasPerPartition (still
deprecated since 1.1.0)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
testBuildConvertsDeprecatedIngestionFields was added when the builder auto-routed legacy ingestion fields into ingestionConfig. The previous commit reverted that rewrite, so the test now contradicts the reverted behavior. testBuildOmitsDefaultDeprecatedFieldsFromSerializedJson remains valid because it depends on @JsonInclude(NON_DEFAULT) on the beans, not on the builder's ingestion routing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The CI failure surface revealed that Pinot's own integration tests (CommitTimeCompactionIntegrationTest, dedup/upsert tests, controller periodic tasks) actively use deprecated table-config setters (setReplicaGroupStrategyConfig, setReplicasPerPartition, etc.). Hard ERROR rejection on create blocks every one of those tests with 400. Demote classifySeverity so every parseable @DeprecatedConfig.since returns WARNING. Only an unparseable since (a code-side annotation bug) remains ERROR. The validator still surfaces every deprecated key as a warning on both create and update — just no longer rejects. Hard ERROR enforcement (the originally-designed older-than-current threshold) can ship in a follow-up PR after the test base and TableConfigBuilder migrate off the deprecated paths. - DeprecatedTableConfigValidationUtils.classifySeverity: drop current-major-minor comparison; always return WARNING for parseable since - DeprecatedTableConfigValidationUtilsTest: rewrite tests asserting ERROR semantics to assert WARNING semantics; consolidate the two current-version tests into one soft-launch test - PinotTableRestletResourceTest.testReportsDeprecatedConfigOnCreate- AndOnUpdateAsWarning: assert deprecationWarnings field on success response instead of 400 rejection - TableConfigsRestletResourceTest: same shape change for the create + update tests on /tableConfigs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restore the streamConfigs @DeprecatedConfig rule now that the validator soft-launches parseable deprecations as warnings instead of rejecting builder-produced legacy configs. Teach update diffing to treat legacy fieldConfigList[].indexType as unchanged when the stored JSON has an equivalent singleton indexTypes array, preserving idempotent updates after @JsonIgnore serialization. Keep the pauseless / logical-table setStreamConfigs(null) regression fix from the amended commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ard, default-value test Three review findings addressed: 1. validate endpoints no longer echo the raw request body (which can carry stream credentials) into 400 responses or controller logs. Both PinotTableRestletResource.checkTableConfig and TableConfigsRestletResource. validateConfig now use "Invalid ... json string. " + e.getMessage() instead of String.format with the full tableConfigStr. 2. TableConfigsRestletResource.validateConfig now runs the permission check BEFORE validateNoDeprecatedConfigs (which reads stored configs from ZK). Prevents an unauthenticated caller from probing table existence via the deprecation-diff response shape, and matches the ordering already used in PinotTableRestletResource.checkTableConfig. setTableName is still deferred until after validateConfig because it has the side effect of overwriting the embedded schema name, which would mask the schema-mismatch check. 3. isJacksonDefault is exposed as a package-private static method, and a new regression test testEveryAnnotatedGetterHasJavaZeroDefault iterates every discovered @DeprecatedConfig rule, instantiates the leaf bean via its no-arg constructor, invokes the leaf getter, and asserts the Java default value is treated as default by isJacksonDefault. Locks in the contract so a future contributor who annotates a non-zero-default getter fails at test time rather than silently suppressing diffs at runtime. The rule record now carries the owning bean class + leaf getter Method for tests to introspect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After rebase onto upstream/master, Jackson's default field ordering changed and the integration tests that assert byte-exact response JSON broke. Explicit @JsonPropertyOrder(unrecognizedProperties, deprecationWarnings, status) makes the wire shape deterministic across Jackson versions and keeps the pre-deprecationWarnings layout that existing clients depend on. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
05def21 to
02b0867
Compare
Summary
Reject explicit use of deprecated table-config keys on both create and update, driven by a single source of truth on the SPI getters instead of a hand-maintained rule list.
@DeprecatedConfig(replacement, since)annotation inpinot-spi. Placed on the deprecated getter;sinceis the Pinot release the field was first marked@Deprecated.DeprecatedTableConfigValidationUtilsdiscovers rules at class-load via a Jackson-aware reflection walk overTableConfig(recursing into nestedBaseJsonConfigtypes,Collection<X>,Map<?,V>, honoring@JsonPropertyrenames). The hand-maintained list is gone.since.major.minorequals the runningPinotVersion.major.minoris reported as a warning (one-release grace period). Older rules are errors that block the request. Unknown current version → safe default of error./tables/{name}, PUT/tableConfigs/{name}, validate POSTs against existing tables) diff the incoming JSON against the byte-faithful stored ZNRecord JSON (new helperTableConfigSerDeUtils.toRawJsonNode+ZKMetadataProvider.getTableConfigZNRecord) — not againstexistingConfig.toJsonNode(), which would silently strip@JsonIgnore-d /@JsonInclude(NON_DEFAULT)deprecated keys and turn every legacy PUT into a false positive. Re-submitting an unchanged legacy value is a no-op; only newly introduced or value-changed deprecated paths fire.deprecationWarnings: List<String>field onConfigSuccessResponseand the validate endpoint JSON. Errors continue to throw400.Deprecated table-config keys covered
Sorted from earliest deprecation to latest. On the current
1.6.0-SNAPSHOTrelease line, everything older than1.6is an error;1.6.0deprecations are warnings (one-release grace period).routing.routingTableBuilderNamerouting.segmentPrunerTypesandrouting.instanceSelectorTypetableIndexConfig.streamConfigsingestionConfig.streamIngestionConfig.streamConfigMapssegmentsConfig.segmentPushFrequencyingestionConfig.batchIngestionConfig.segmentIngestionFrequencysegmentsConfig.segmentPushTypeingestionConfig.batchIngestionConfig.segmentIngestionTypefieldConfigList[*].indexTypefieldConfigList[].indexTypestableIndexConfig.jsonIndexColumnstableIndexConfig.jsonIndexConfigssegmentsConfig.replicasPerPartitionsegmentsConfig.replicationinstanceAssignmentConfigMap[*].replicaGroupPartitionConfig.minimizeDataMovementsegmentsConfig.replicaGroupStrategyConfigsegmentAssignmentConfigMapsegmentsConfig.minimizeDataMovementinstanceAssignmentConfigMapupsertConfig.enableSnapshotupsertConfig.snapshotupsertConfig.enablePreloadupsertConfig.preloadupsertConfig.allowPartialUpsertConsumptionDuringCommitingestionConfig.streamIngestionConfig.parallelSegmentConsumptionPolicydedupConfig.enablePreloaddedupConfig.preloaddedupConfig.allowDedupConsumptionDuringCommitingestionConfig.streamIngestionConfig.parallelSegmentConsumptionPolicytableIndexConfig.createInvertedIndexDuringSegmentGenerationsincewas determined per field by walkinggit log/git tag --containsagainst the upstream apache/pinot history to find the first release tag that ships the@Deprecatedannotation (or the original@deprecatedJavadoc when that came first).Behavior
400; warnings → server WARN log +deprecationWarningsfield.TableConfigBuilder.build()now converts the deprecated_segmentPushType/_segmentPushFrequencysetters into moderningestionConfig.batchIngestionConfig.segmentIngestionType/Frequency, so existing tests and tools that use the builder produce create payloads that pass validation.Testing
./mvnw -pl pinot-spi,pinot-common,pinot-controller -am -Dtest='DeprecatedTableConfigValidationUtilsTest+TableConfigSerDeUtilsTest+TableConfigBuilderTest+PinotTableRestletResourceTest#testRejectsDeprecatedConfigOnCreateAndOnUpdateWhenNewlyIntroduced+PinotTableRestletResourceTest#testUpdateAllowsUnchangedLegacyDeprecatedConfig+TableConfigsRestletResourceTest' test./mvnw -pl pinot-spi,pinot-common,pinot-controller spotless:apply checkstyle:check license:checkCoverage includes: annotation discovery walk against
TableConfig, diff filtering on update, version-based severity classification, raw-JSON preservation across@JsonIgnore/@JsonInclude(NON_DEFAULT)getters, and round-trip rejection/acceptance through the controller REST endpoints.